fix: prompt=none shows a login screen#1361
Conversation
andyzickler
force-pushed
the
feat/silent-login
branch
from
cc78691 to
29a0472
Compare
dopry
force-pushed
the
feat/silent-login
branch
from
29a0472 to
5e48ee8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1361 +/- ##
==========================================
+ Coverage 97.54% 97.56% +0.01%
==========================================
Files 32 32
Lines 2120 2132 +12
==========================================
+ Hits 2068 2080 +12
Misses 52 52 ☔ View full report in Codecov by Sentry. |
andyzickler
force-pushed
the
feat/silent-login
branch
from
29f384e to
a592988
Compare
dopry
force-pushed
the
feat/silent-login
branch
from
a592988 to
5681e2e
Compare
|
This PR was a bit optimistic and naïve. Tests written via spec passed, but not in real world testing with the ldp and rp apps. Future implementation likely needs to implement oauthlib's |
|
I think it was on the right track. Let's isolate the dispatch fixes so someone could at least in theory implement validate_silent_login |
dopry
force-pushed
the
feat/silent-login
branch
from
5681e2e to
c0cb28f
Compare
dopry
changed the title
dopry
force-pushed
the
feat/silent-login
branch
5 times, most recently
from
81313bd to
404e248
Compare
dopry
force-pushed
the
feat/silent-login
branch
from
c00b681 to
36121c5
Compare
for more information, see https://pre-commit.ci
|
@n2ygk @tonial I'd love to get a review from you guys on this. I've been working on it with @andyzickler. This bug is blocking an SSO implementation for me I'd really like to complete. |
tonial
left a comment
tonial
left a comment
There was a problem hiding this comment.
I made a few comments on the tests, but the implementation seems correct to me.
Great addition !
| scheme, netloc, path, params, query, fragment = urlparse(response["Location"]) | ||
| parsed_query = parse_qs(query) | ||
|
|
||
| self.assertIn("login_required", parsed_query["error"]) |
There was a problem hiding this comment.
You could use self.assertEqual(parsed_query, expected_dict) to be sure we don't add anything else.
| "response_type": "code", | ||
| "state": "random_state_string", | ||
| "scope": "read write", | ||
| "redirect_uri": "http://example.org", |
There was a problem hiding this comment.
Maybe add a query parameter, or add another test with a query parameter in the redirect_url to test the seperator computed line 268 in handle_no_permission()
4 participants
Fixes #1268
Description of the Change
Fix bug preventing support for Silent Authentication. If an unauthorized request to
AuthorizationViewwith a query parameter that containsprompt=nonehappens, then we will redirect with an error code oflogin_requiredotherwise everything will proceed as before.See https://auth0.com/docs/authenticate/login/configure-silent-authentication#error-responses
and https://openid.net/specs/openid-connect-core-1_0.html#AuthError
fully supporting prompt=none will require implementing validate_silent_login in the validator. this doesn't implement that, but will allow people to implement it if they want until we can implement a good implementation for DOT.
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS